Skip to content

[material-ui][Chip] Add slots and slotProps #46098

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented May 7, 2025

This PR adds slots and slotProps to Chip component

@sai6855 sai6855 changed the title Chip-slots [material-ui][Chip] Add slots and slotProps May 7, 2025
@sai6855 sai6855 added component: chip This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material enhancement This is not a bug, nor a new feature labels May 7, 2025
@mui-bot
Copy link

mui-bot commented May 7, 2025

Netlify deploy preview

https://deploy-preview-46098--material-ui.netlify.app/

@mui/material/Chip: parsed: +2.15% , gzip: +2.04%
@mui/material/Autocomplete: parsed: +0.22% , gzip: +0.18%
@mui/material: parsed: +0.06% , gzip: +0.05%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 3f3da41

@sai6855 sai6855 marked this pull request as draft May 7, 2025 07:42
@sai6855 sai6855 requested a review from siriwatknp May 7, 2025 11:47
@sai6855 sai6855 marked this pull request as ready for review May 7, 2025 11:47
},
ownerState,
ref: handleRef,
shouldForwardComponentProp: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
shouldForwardComponentProp: true,

ChipRoot comes from div, no need to forward component prop

Copy link
Contributor Author

@sai6855 sai6855 May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The component prop was forwarded because, when shouldForwardComponentProp is set to false, the component prop returned from useSlot('root') gets removed in the useSlot.ts file and replaced with the as prop here.

While this behavior works fine for components that do not rely on component-specific logic, it breaks components like Chip, which access the component prop directly in their logic — for example, this line. Removing the component prop in such cases causes test failures.

To fix this, I added shouldForwardComponentProp: true to ensure that the component prop is preserved and Chip continues to behave correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Can you add a comment above the line shouldForwardComponentProp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, added here 29b9ea4

@siriwatknp
Copy link
Member

@sai6855 Can you check the CIs? Some are failing.

@sai6855 sai6855 requested a review from siriwatknp May 20, 2025 08:39
label: {
expectedClassName: classes.label,
},
},
Copy link
Member

@siriwatknp siriwatknp May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the comment about the shouldForwardComponentProp. I think we need to add more tests.

  • With clickable: true and a custom component prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it('should render link with the button base', () => {
const { container } = render(<Chip component="a" clickable label="My text Chip" />);
expect(container.firstChild).to.have.class('MuiButtonBase-root');
expect(container.firstChild).to.have.tagName('a');
});
There is test which tests with same props combination as mentioned. is this test sufficient or do you have any other test in mind?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: chip This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants